Skip to content

ui pattern failure tests #524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 10, 2024
Merged

ui pattern failure tests #524

merged 6 commits into from
Jun 10, 2024

Conversation

lordshashank
Copy link
Contributor

@lordshashank lordshashank commented May 26, 2024

  • resolves Run the error UI tests in a new CI job #495
  • implements function for specific ui pattern failure tests here
  • Run this test as ci job in failures.yml
  • Refactored various function names as per functionality.
  • Added comments of parms of test_rustc_inner

@lordshashank
Copy link
Contributor Author

lordshashank commented May 26, 2024

@antoyo after ideating for hours on how to proceed and looking for various options like implementing completely new function for this, including simple bool param in test_rustc_inner for this, making global bool variable to include/exclude these tests, etc.
I thought best option would be to include dynamic boxed closure callback function as parameter to test_rustc_inner function, this would enable us to implement separate tests/ci jobs for other checks pattern in different test suites if we implement in future.
Let me know if you have any better way and if this didn't worked correctly, thanks.

@antoyo
Copy link
Contributor

antoyo commented May 27, 2024

To make this useful, we would need the test to fail when there's an ICE (internal compiler error).

We can see in the logs that some tests have an ICE:

2024-05-26T23:54:45.8957411Z thread 'rustc' panicked at compiler/rustc_trait_selection/src/traits/select/confirmation.rs:795:50:
2024-05-26T23:54:45.8957573Z future has no bound vars
2024-05-26T23:54:45.8957837Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2024-05-26T23:54:45.8957843Z 
2024-05-26T23:54:45.8958035Z error: the compiler unexpectedly panicked. this is a bug.

Maybe we could grep for the compiler unexpectedly panicked and fail the test in this case.
We would then need a file for to ignore the tests that are known to produce an ICE.

It would also be interesting to check if this is a good strategy. Perhaps some tests also produce an ICE with the LLVM codegen. Perhaps there is an annotation for the tests that produce an ICE even with LLVM? Perhaps it is known-bug?

@lordshashank
Copy link
Contributor Author

@antoyo where are these logs from? From workflow run I can see that around 474 ui-pattern-failure-test passed. Do you mean that some of them have an ICE and we should fail them manually?

@antoyo
Copy link
Contributor

antoyo commented May 27, 2024

To see those logs, I went to the failure workflow and clicked on "View raw logs" (because the logs are too long to be shown on the previous page).

@antoyo
Copy link
Contributor

antoyo commented May 27, 2024

Yeah, we should fail the CI when there's an ICE, but please consider the notes I mentioned above.

@lordshashank
Copy link
Contributor Author

lordshashank commented May 28, 2024

@antoyo what do you mean by

we would need the test to fail when there's an ICE

all the cases for ICE (with "the compiler unexpectedly panicked") that I see in logs are for the ui tests that are already failing.

@antoyo
Copy link
Contributor

antoyo commented May 28, 2024

Indeed, but the overall CI task does not fail because we eat the exit code.
I would want the CI task to fail when an ICE is detected (except if it is expected: see notes above).

@lordshashank
Copy link
Contributor Author

@antoyo I checked few of them manually, could get "known-bug" in the one you shared above only. Could you tell me how can I grep for the compiler unexpectedly panicked to get the tests that are giving this as all this is being run through exec_command function, not sure how to get logs to grep this.

@antoyo
Copy link
Contributor

antoyo commented May 30, 2024

You can see the logs by going to this page, then click on the gear at the top right and either select "View raw logs" or "Download log archive".
You need to do this since the logs are too long to be shown in the page.

@lordshashank
Copy link
Contributor Author

Ah! I already did that, is there only manual way to get the tests with ICE from finding the logs, I was thinking of some programmatic way. (there are many, approx 100 such cases in logs)

@antoyo
Copy link
Contributor

antoyo commented May 30, 2024

You mean, finding the name of those tests?

@lordshashank
Copy link
Contributor Author

lordshashank commented May 30, 2024

yes (name or file) , cause then only i would be able to create txt file to omit them or check for "known-bug", right?

@antoyo
Copy link
Contributor

antoyo commented May 30, 2024

Yes.

I do not know any programmatic way to do this.

@GuillaumeGomez mentioned to me the results are in JSON at some point, but we don't know if we could get the JSON.

@GuillaumeGomez
Copy link
Member

To give more details: there is the tester which runs ui test with the --output-format json, then parses this output and checks if it's expected or not. I don't think this tester itself has an option to output json though.

@lordshashank
Copy link
Contributor Author

Ok, so had to do a lot here.
First I looked for the ICE failures using script on logs downloaded, checked for known bug found just one case you shared. Was reluctant to check for any other error pattern as didn't seem universal here, and could have led to removal of some successfull test as once happened before.
Next, while getting the compiler unexpectedly panicked found one case in run-make tests as well here. But I guess this was expected here.
Then, had do callback for tests, improved some function naming there (let me know if thats an issue).
For CI failure, had to grep from logs, found no other way, let me know if you have one.
Also, @antoyo I mistakingly raised PR to this branch rather than yours, let me know if I have close this and raise there.
Huh 😅
Will look why the runs failed next.
Thanks.

@lordshashank
Copy link
Contributor Author

@antoyo ICE tests should get removed through the callback here, I tried to log them here and the failing tests were not there. Still the CI run took those tests leading to failing of CI. Could you help me know what am I missing here?

@antoyo
Copy link
Contributor

antoyo commented Jun 6, 2024

From what I can see, the command ./y.sh test --release --clean --build-sysroot --test-failing-ui-pattern-tests will not run the tests from the file failing-ice-tests.txt. If we still see failures of tests like tests/ui/sepcomp/sepcomp-statics.rs, they happen before we run this command.

In fact, we see them in the command ./y.sh test --release --clean --build-sysroot --test-failing-rustc because the file tests/failing-ui-tests.txt lists those same tests tests/ui/sepcomp/sepcomp-statics.rs.

(You can search for "build system" in the output to see the command that is ran about 14 lines above "build system".)

I tried to log them here and the failing tests were not there

For the record, this is normal that you don't see them here because they were removed by the call to prepare_files_callback_remove.

All of that to say, the CI fails because some other tests (for instance tests/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs gives an ICE), but these tests are not removed (currenty because the callback should_run_test_callback returns true), so I believe you need to add these tests to the file failing-ice-tests.txt.

@lordshashank
Copy link
Contributor Author

ok, so it was that. Actually I looked at logs and saw same files that I omitted in failing-ice-tests.txt , thus thought they were not getting omitted, but actually they were from another ci task, sorry.
I guess CI has passed but I got a mail of it failing, not sure why. @antoyo let me know if anything else is needed here.

Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few changes I'd like to see.

Thanks for your work!

@lordshashank
Copy link
Contributor Author

@antoyo added the bool function, let me know if anything else is needed now. Also updated description.

@lordshashank
Copy link
Contributor Author

@antoyo ig we are good to go now.

Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these changes, it will be good to go.
Thanks for the work!

@lordshashank
Copy link
Contributor Author

LFG 🚀

@antoyo antoyo merged commit a63b83e into rust-lang:master Jun 10, 2024
34 checks passed
antoyo pushed a commit to zedar/rustc_codegen_gcc that referenced this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run the error UI tests in a new CI job
3 participants